Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto-common: migrate to hybrid-array; MSRV 1.65 #1319

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

tarcieri
Copy link
Member

Replaces generic-array with hybrid-array, which is built on a combination of typenum and const generics, providing a degree of interoperability between the two systems.

hybrid-array is designed to be a largely drop-in replacement, and the number of changes required to switch are relatively minimal aside from some idiosyncracies.

@tarcieri tarcieri requested a review from newpavlov May 30, 2023 23:35
crypto-common/src/lib.rs Outdated Show resolved Hide resolved
crypto-common/src/lib.rs Outdated Show resolved Hide resolved
@tarcieri tarcieri force-pushed the crypto-common/hybrid-array branch 3 times, most recently from 5e808f0 to 0856ad0 Compare May 31, 2023 02:57
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but it would be nice to see migration of the other trait crates as well to be safe against potential unforeseen issues.

@tarcieri tarcieri force-pushed the crypto-common/hybrid-array branch 3 times, most recently from 5a6327f to 0c9a9bb Compare September 5, 2023 01:34
tarcieri added a commit to RustCrypto/utils that referenced this pull request Sep 5, 2023
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
tarcieri added a commit to RustCrypto/utils that referenced this pull request Sep 5, 2023
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
@tarcieri
Copy link
Member Author

tarcieri commented Sep 5, 2023

Since block-buffer depends on crypto-common, I've opened the next step of this migration in the utils repo: RustCrypto/utils#944

tarcieri added a commit to RustCrypto/utils that referenced this pull request Sep 5, 2023
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
tarcieri added a commit to RustCrypto/utils that referenced this pull request Sep 5, 2023
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
tarcieri added a commit to RustCrypto/utils that referenced this pull request Sep 5, 2023
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
@newpavlov
Copy link
Member

generic-array v1.0 has implemented Try/From for arrays using const generics and simplified the ArrayLength trait. Is this PR still relevant in the light of it?

@tarcieri
Copy link
Member Author

tarcieri commented Sep 13, 2023

Yes, hybrid-array is natively built on const generic arrays at its core. Array is a newtype of the inner array type, which is pub.

generic-array instead has an opaque inner type and uses large amounts of unsafe code to achieve its goals. It has wrappers to use const generics which use a similar technique to hybrid-array (and thus have similar limitations), but if you use that approach like hybrid-array has done you can go all-in and get rid of (most of) the unsafe code.

@newpavlov
Copy link
Member

Ok. Should we migrate to generic-array v1.0 in the next release cycle first or would you prefer to try hybrid-array migration without it?

@tarcieri
Copy link
Member Author

Personally I'd prefer to go straight to hybrid-array and have already done some of the work, see also RustCrypto/utils#944

I can do cipher and digest next.

tarcieri added a commit to RustCrypto/utils that referenced this pull request Oct 8, 2023
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
tarcieri added a commit to RustCrypto/utils that referenced this pull request Oct 8, 2023
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
tarcieri added a commit to RustCrypto/utils that referenced this pull request Oct 8, 2023
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
tarcieri added a commit to RustCrypto/utils that referenced this pull request Oct 8, 2023
Builds on RustCrypto/traits#1319.

Migrates the following crates away from using `generic-array` to using
`hybrid-array` instead:

- `block-buffer`
- `block-padding`
- `dbl`
- `inout`
@tarcieri tarcieri force-pushed the crypto-common/hybrid-array branch 2 times, most recently from 2854b76 to 11330b0 Compare October 26, 2023 01:30
@tarcieri tarcieri changed the title [WIP] crypto-common: migrate to hybrid-array; MSRV 1.65 crypto-common: migrate to hybrid-array; MSRV 1.65 Oct 26, 2023
@tarcieri
Copy link
Member Author

The minimal-versions failure seems to be a nightly regression and probably impacts master as well

Replaces `generic-array` with `hybrid-array`, which is built on a
combination of `typenum` and const generics, providing a degree of
interoperability between the two systems.

`hybrid-array` is designed to be a largely drop-in replacement, and the
number of changes required to switch are relatively minimal aside from
some idiosyncracies.
@tarcieri tarcieri marked this pull request as ready for review October 26, 2023 15:22
@tarcieri
Copy link
Member Author

@newpavlov this is ready now

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also change bound for the BlockSize sealed impl from Self: IsLess<U256> + IsGreater<U1> to Self: IsLess<U256> + IsGreater<U0>? I intended to do it in #1174.

crypto-common/src/lib.rs Outdated Show resolved Hide resolved
@tarcieri tarcieri merged commit 766c138 into master Oct 26, 2023
86 checks passed
@tarcieri tarcieri deleted the crypto-common/hybrid-array branch October 26, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants